Samuel100/sdkv2 rust - #836
Conversation
Replace the owned-Self FoundryLocalManager::create with an Arc<Self> that shares one process-wide instance via a static OnceLock<Mutex<Weak<...>>>. While any handle is alive, callers share the same instance; when the last Arc drops, NativeManager::Drop runs teardown (Manager_Shutdown + Manager_Release) while the ORT runtime is still alive and before the library's C++ static destructors -- restoring singleton semantics without the atexit hook that caused the WebGPU ReleaseEpFactory double-unregister (ORT #29206). Use OnceLock to wrap the Mutex (const Weak::new() needs Rust 1.73, above the crate's 1.70 MSRV). Update stale atexit/singleton doc comments in manager.rs, foundry_local_manager.rs, and docs/api.md. Keep the test helper's &'static signature by holding a process-lifetime OnceLock<Arc<...>>, so all existing call sites compile unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…elease Fix a use-after-free reachable from safe code: Model, the OpenAI clients, and sessions held only a raw flModel*/flSession* plus Arc<Api> (which keeps just the shared library loaded), so dropping the last Arc<FoundryLocalManager> released the native catalog and model handles while those derived objects were still alive. A natural factory pattern (create manager, get a client, return it) would dangle. This was latent under the old leaked &'static singleton and became reachable once teardown moved onto last-Arc Drop. Thread a strong Arc<NativeManager> keep-alive through NativeModel, NativeCatalog, and NativeSession so every derived handle keeps the native manager (and thus the catalog/model handles it owns) alive until the handle itself is dropped. The keep-alive targets NativeManager (a leaf that owns no Rust wrappers), so there is no reference cycle. Also add NATIVE_LIFECYCLE, a Mutex<()> held across both Manager_Create and Manager_Release, to close the create/teardown race: an Arc's strong count reaches zero slightly before Drop finishes Manager_Release, so a concurrent create() could otherwise observe no instance yet be rejected by the native single-instance guard. Validated: cargo fmt/check/clippy/doc clean (lib, tests, examples). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
origin/main #746 (Add speech result types) inserted GetSpeechSegment and GetSpeechResult into flItemApi between GetToolResult and GetMetadata. Because ffi.rs mirrors the vtable positionally via #[repr(C)], the old layout left GetMetadata/GetMutableMetadata/GetQueue and all ItemQueue_* slots shifted by two pointers — so calls like ItemQueue_Push/TryPop (used by streaming) would misdispatch to the wrong native function against a core built from the new header. Add the two function-pointer slots in the matching position, plus the supporting flSpeechWord/flSpeechSegmentData/flSpeechResultData structs, flSpeechSegmentKind, the FOUNDRY_LOCAL_ITEM_SPEECH_SEGMENT/RESULT item-type constants, and the DURATION/CONFIDENCE_UNSET sentinels. API version is unchanged (still 1). Validated: cargo fmt/check/clippy clean (lib, tests, examples). Vtable order now matches foundry_local_c.h flItemApi field-for-field (31 entries). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
origin/main #746 changed the native audio path to emit SPEECH_SEGMENT items (streaming) and a SPEECH_RESULT item (final), replacing the TextItem outputs. LiveAudioTranscriptionSession uses that native path but read results only via read_text_item / item_text (ITEM_TEXT only), so against the new core it silently produced empty streaming results and an empty final transcript. Add read_speech_segment / read_speech_result_text helpers (via the new GetSpeechSegment / GetSpeechResult accessors) and wire them into the streaming trampoline and final-transcript aggregation, with a TEXT fallback so the OpenAI-JSON path and older cores keep working. Segment timing (ms→s) and PARTIAL/FINAL state are mapped onto the existing response envelope. AudioClient::transcribe / transcribe_streaming are unaffected — they use the OpenAI-JSON path, which still returns OPENAI_JSON-tagged TEXT items. Validated: cargo fmt/check/clippy clean (lib, tests, examples). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR ports the Foundry Local Rust SDK (sdk_v2/rust/) onto the new C++ Core ABI exposed through foundry_local_c.h. It rebuilds the public surface (FoundryLocalManager, Catalog, Model, OpenAI-style chat/embedding/audio clients, and a live-audio streaming session) on top of thin extern "system" FFI wrappers, with native-handle lifetimes anchored to a shared Arc<NativeManager>, and adds a single-binary integration test suite plus generated API docs.
Changes:
- New FFI-backed core wrappers (
detail/{api,native,manager,session,items,model,info,...}) that mirror the C ABI's ownership model (Manager owns Catalog/Models; handles keep the Manager alive; item-queue/request ownership transfer rules). - New OpenAI-compatible clients (
openai/{chat_client,embedding_client,audio_client,live_audio_session}) and public types, re-exported fromlib.rs. - A consolidated integration test binary (
tests/integration/*), examples,Cargo.toml/build.rsupdates, and API reference docs.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
sdk_v2/rust/src/openai/live_audio_session.rs |
Live audio streaming session; exposes push_queue_capacity/bits_per_sample options that aren't wired to the native layer (commented). |
sdk_v2/rust/src/detail/{api,native,manager,session,items,model,info}.rs |
Core FFI wrappers and native lifetime/ownership handling; verified against the C ABI and stored conventions. |
sdk_v2/rust/src/openai/{chat_client,embedding_client,audio_client,mod}.rs |
OpenAI-compatible clients and response/streaming handling. |
sdk_v2/rust/src/{lib,catalog,configuration,foundry_local_manager,types,error}.rs |
Public API surface and re-exports (Model, DownloadBuilder, etc.). |
sdk_v2/rust/tests/integration/common/mod.rs |
Shared test config; logsDir still points at the legacy sdk/rust tree (commented). |
sdk_v2/rust/tests/integration/{main,manager,model,web_service,live_audio,embedding_client,chat_client}_test.rs |
New consolidated integration tests; minor duplicate/mislabeled log line in chat test (commented). |
sdk_v2/rust/GENERATE-DOCS.md, sdk_v2/rust/docs/api.md |
Doc generation guide and API reference; reference the legacy sdk/rust path and a nonexistent ModelVariant type (commented). |
sdk_v2/rust/Cargo.toml, sdk_v2/rust/build.rs |
Package metadata and native-artifact build script (verified). |
|
All Copilot comments look actionable as well |
Docs/paths (Copilot): - test logsDir, GENERATE-DOCS.md, docs/api.md: legacy sdk/rust -> sdk_v2/rust - GENERATE-DOCS.md: replace nonexistent ModelVariant row with DownloadBuilder Tests (Copilot): - chat_client_test: drop the redundant, mislabeled "REST response" print Model API (nenad1002): - unload/remove_from_cache now return Result<()> (no empty-String sentinel) - ModelKind stores Arc<VariantData> to avoid deep VariantData/NativeModel clones - create_chat/audio/embedding_client read selected_variant() once so a concurrent select_variant_by_id can't mismatch id vs native - fix the single-variant select error text; method name no longer misstated Catalog (nenad1002): - update_models doc states plainly it is a no-op (native self-refreshes) Live audio (Copilot + nenad1002): - remove unwired bits_per_sample and push_queue_capacity options and the false backpressure claim (append() is unbounded); update the live-audio test - add Drop to LiveAudioTranscriptionSession: best-effort mark the input queue finished via get_mut so the blocking worker drains and the native session is released if the session is dropped without stop() Configuration (nenad1002): - logger() doc states plainly it is not wired to native (no logger-callback ABI; use log_level/logs_dir), kept for forward compatibility build.rs (nenad1002, nit): - integrity guard: reject empty payloads and require the expected native file to be present after extraction. Cryptographic SHA-512-vs-registration is feed- specific (nuget.org vs Azure DevOps) and left as future hardening. Validated: cargo fmt/check/clippy/doc clean (lib, tests, examples). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ports the modality-agnostic inference surface from the C++/C#/Python/JS bindings to the Rust SDK, following idiomatic Rust: - Pure-data owned values: `Item` (enum + Text/Message/Tensor/Image/Audio/ ToolCall/ToolResult/Speech* variants and supporting types), `Request`, `Response` — all Send+Sync+Clone with no native handle. - Handle-backed types shared via `Arc`: `Session` (+ typed `ChatSession`, `EmbeddingsSession`, `AudioSession` which Deref to `Session`) and `ItemQueue`. - Streaming via `futures_core::Stream` (`ItemStream`) instead of a callback setter; dropping the stream cancels generation. - `process_request` is cancel-on-drop: a `CancelGuard` fires the native `Request_Cancel` if the future is dropped before completion (e.g. via `tokio::time::timeout` / `select!`), matching every other binding's `Request::Cancel` while staying idiomatic. - Convenience helpers: `ChatSession` tool + turn management, `EmbeddingsSession::embed`/`embed_batch`, `AudioSession::transcribe`. Adds native bridge (`detail/items.rs`, `detail/session.rs`), FFI param/dtype constants, a low-level example, integration tests, and API docs. Renames the OpenAI facade's `FinishReason` re-export to `ChatFinishReason` to avoid colliding with the core inference `FinishReason`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
…ts (Rust)
Correct the Rust OpenAI direct chat client so its request JSON matches the
authoritative native `*-json` contract (chat_completions_converter.cc):
- response_format keys use snake_case (`json_schema`, `lark_grammar`) — the
previous camelCase keys were silently dropped by the native converter.
- tool_choice emits plain strings ("none"/"auto"/"required") and a nested
`{"type":"function","function":{"name":…}}` for a named function, matching
what the converter parses. Add regression unit tests locking in the shapes.
Deprecate the OpenAI direct clients (ChatClient, AudioClient, EmbeddingClient,
LiveAudioTranscriptionSession) and their factories with
`#[deprecated(since = "2.0.0")]`, pointing users to the Session API
(ChatSession/AudioSession/EmbeddingsSession), matching the C#/JS/Python SDKs.
Facade modules, re-exports, examples and integration tests opt out via
`#[allow(deprecated)]` to keep the `-D warnings` build green. Update docs/api.md.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
Migrate the bundled Rust SDK examples off the deprecated OpenAI-facade chat client onto the idiomatic Session/Item/Request/Response API: - chat_completion.rs: ChatSession::new + set_options; sync process_request -> response.text(); streaming via process_streaming_request + as_text(). - interactive_chat.rs: stateful loop; system prompt seeded only when turn_count() == 0; no manual message history. - tool_calling.rs: register the tool once via add_tool_definition; first turn tool_choice=Required streams complete Item::ToolCall values; feed back only Item::tool_result; second turn tool_choice=None for the final answer. Removes the manual streamed-tool-call delta reassembly. Drops #![allow(deprecated)] from all three; they no longer touch the facade. Verified: cargo clippy --all-targets -- -D warnings, cargo fmt --check, cargo build --examples, cargo test --lib (14/14) all clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated 20 comments.
Comments suppressed due to low confidence (4)
sdk_v2/rust/tests/integration/session_test.rs:110
- The native core refuses to unload a model while any session still references it.
sessionremains alive until the end of this test, so thisunload()is guaranteed to return “session(s) still using it” and fail the test. Drop the session first.
sdk_v2/rust/tests/integration/session_test.rs:131 - The native core refuses to unload a model while any session still references it.
sessionremains alive until the end of this test, so thisunload()is guaranteed to return “session(s) still using it” and fail the test. Drop the session first.
sdk_v2/rust/tests/integration/session_test.rs:169 - The embeddings session still owns a live native session here, and the core rejects unloading models with live sessions. Drop
sessionbefore unloading so this test can complete successfully.
sdk_v2/rust/examples/tool_calling.rs:144 sessionstill owns the native chat session here, so the core rejects this unload with “session(s) still using it.” Drop the session before model cleanup.
model.unload().await?;
Correctness: - Drop sessions before unloading models in examples and tests (native rejects unload while a session is still using the model) - Send an empty C string for a null ToolDefinition description (the C ABI rejects a null description) - Propagate EP discovery failures instead of collapsing to the "download all" path; validate EP names for interior NUL bytes - Make an explicit library_path override authoritative: error instead of falling through to the env/exe/system search - Buffer SSE bytes and parse only complete lines in the web-service test - Hard-fail (not skip) when a live-audio session fails to start - Drive build.rs off CARGO_CFG_TARGET_* so RID/extension/link selection is correct under cross-compilation Concurrency: - Serialise web-service start/stop with an async mutex - Add a per-session op-lock; mutators self-lock and the streaming paths hold it across install->process->uninstall (raw process_request stays unlocked to avoid reentrancy) - Serialise the web-service integration tests with a shared async guard Packaging: - Commit a crate-local deps_versions.json so the pinned native versions ship in the published crate, and add a build.rs guard that fails on drift from the canonical sdk_v2/deps_versions.json Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
… wrapper The process-wide singleton tracked only a Weak<FoundryLocalManager> (the outer wrapper). Derived handles (Model/Session/Catalog) keep only the inner Arc<NativeManager> alive, so dropping the outer wrapper while holding a derived handle expired the singleton slot even though the native flManager lived on. The next create() then attempted a second Manager_Create, which the core rejects with INVALID_USAGE. Track both the outer wrapper and the inner native manager as weak refs in a Mutex<SharedInstance>. When the outer is gone but the native is still alive, rebuild a wrapper around the live native (wrap_existing) instead of creating a second native manager. wrap_existing also seeds the URL cache from the native manager so a rebuilt wrapper correctly reports an already-running web service. Add regression tests as separate integration binaries that own every handle: - singleton_lifetime: drop the outer wrapper while holding a Model, then recreate. - web_service_lifetime: start/serve/stop the web service over HTTP and confirm wrap_existing surfaces a running service. Both skip gracefully when the native core is unavailable or built without web-service support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
Rust SDK updated to new C++ Foundry Local Core ABI.